Add ElicitAsync<T> (#630) #715
Conversation
- Refactored `ElicitResult` to a generic class `ElicitResult<T>` for typed content. - Added `ElicitAsync<T>` method in `McpServerExtensions.cs` to request user input and construct schemas based on type `T`. - Implemented schema building logic to handle primitive types and enums, ignoring unsupported types. - Introduced `ElicitationTypedTests.cs` for testing the new elicitation functionality with typed forms. - Verified naming policies in tests to ensure correct serialization casing. - Defined `SampleForm` and `CamelForm` classes for expected input shapes, including unsupported properties for schema testing. - Created JSON serialization contexts for both forms using source generation for improved performance.
|
@stephentoub Could you review this, please? |
…ion. modelcontextprotocol#630 Renamed `BuildRequestSchemaFor<T>` to `BuildRequestSchema<T>`. Updated the implementation to use `CreatePrimitiveSchema` and enhanced type checks for supported primitives with `AIJsonUtilities.CreateJsonSchema`. Streamlined handling of unsupported types by consolidating return statements.
|
@eiriktsarpalis I've made the changes based on your suggestions, utilizing |
|
|
||
| private static ElicitRequestParams.PrimitiveSchemaDefinition? CreatePrimitiveSchema(Type type, JsonSerializerOptions serializerOptions) | ||
| { | ||
| Type t = Nullable.GetUnderlyingType(type) ?? type; |
There was a problem hiding this comment.
Note that nullable variants of primitives generate different schemas, so it would be for the best if the two cases weren't collapsed. In other words, you should be pasing type into the CreateJsonSchema call in line 324.
Simplified the `ElicitAsync` method in `McpServerExtensions.cs` by removing unnecessary JSON object construction and directly deserializing `raw.Content`. Updated `CreatePrimitiveSchema` to use `JsonTypeInfo` for type checking and adjusted return logic for unsupported types. In `ElicitationTypedTests.cs`, modified the `Can_Elicit_Typed_Information` test to account for the new `Created` property in `SampleForm`, increasing the expected property count from 5 to 6. Added assertions for the type and format of the `Created` property and included its deserialization in the test setup.
|
@eiriktsarpalis I reviewed the MCP specification and realized I was mistaken. It does support datetime, but it’s converted into a string type with 'date-time' formatting, consistent with |
|
|
||
| private static ElicitRequestParams.PrimitiveSchemaDefinition? CreatePrimitiveSchema(Type type, JsonSerializerOptions serializerOptions) | ||
| { | ||
| Type underlyingType = Nullable.GetUnderlyingType(type) ?? type; |
There was a problem hiding this comment.
Please remove this normalization step.
There was a problem hiding this comment.
If I remove this, nullable types such as bool? will result in a resolved schema represented as an array with "type": ["boolean", "null"] (instead of "type": "boolean"). This cannot be deserialized into a PrimitiveSchemaDefinition because the converter expects the types to be strings and does not support arrays:
public class Converter : JsonConverter<PrimitiveSchemaDefinition>
{
public override PrimitiveSchemaDefinition? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
// ...
switch (propertyName)
{
case "type":
type = reader.GetString(); // THROWS exception as it is an array for nullable types like bool?
break;To resolve this, I could modify the converter to accommodate nullable types in this way:
case "type":
if (reader.TokenType == JsonTokenType.String)
{
type = reader.GetString();
}
else if (reader.TokenType == JsonTokenType.StartArray)
{
var types = JsonSerializer.Deserialize(ref reader, McpJsonUtilities.JsonContext.Default.StringArray);
if (types is [var nullableType, "null"])
{
type = nullableType;
}
}
break;Does this make sense?
There was a problem hiding this comment.
@eiriktsarpalis any thoughts on this?
There was a problem hiding this comment.
Assuming the elicitation spec permits arrays in type keywords, then I think that would be the right thing to do.
|
|
||
| var jsonElement = AIJsonUtilities.CreateJsonSchema(underlyingType, serializerOptions: serializerOptions); | ||
|
|
||
| if (jsonElement.TryGetProperty("type", out var typeElement)) |
There was a problem hiding this comment.
What if the schema is just {} or true? What if the schema contains a supported type keyword but then also contains other unsupported keywords? Shouldn't we be validating those as well?
…ocol#630 Introduce exception handling in `McpServerExtensions.cs` for unsupported types. Add a test case in `ElicitationTypedTests.cs` to verify exception throwing for unsupported types. Define a new `UnsupportedForm` class with nested properties and include JSON serialization attributes for proper handling.
Introduce validation to ensure only object types are supported for elicitation requests in the `BuildRequestSchema` method. An exception is thrown for non-object types. Update the test suite with a new test case to verify this behavior, ensuring that an exception is raised when eliciting a non-object generic type (e.g., string) and that the elicitation handler is not invoked in this scenario.
ElicitResult<T>for typed content.ElicitAsync<T>method inMcpServerExtensions.csto request user input and construct schemas based on typeT.ElicitationTypedTests.csfor testing the new elicitation functionality with typed forms.SampleFormandCamelFormclasses for expected input shapes, including unsupported properties for schema testing.Motivation and Context
It resolves #630
How Has This Been Tested?
I've added proper unit tests for it.
Breaking Changes
No breaking changes
Types of changes
Checklist
Additional context
No additional notes.